[lldb][Process/FreeBSDKernelCore] Defer actual core loading to DoLoadCore()#186626
[lldb][Process/FreeBSDKernelCore] Defer actual core loading to DoLoadCore()#186626
Conversation
Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
|
@llvm/pr-subscribers-lldb Author: Minsoo Choo (mchoo7) ChangesCurrently Full diff: https://github.com/llvm/llvm-project/pull/186626.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.cpp b/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.cpp
index d1a4a1ebc47d7..ad94c86e5499a 100644
--- a/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.cpp
+++ b/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.cpp
@@ -62,9 +62,8 @@ static PluginProperties &GetGlobalPluginProperties() {
ProcessFreeBSDKernelCore::ProcessFreeBSDKernelCore(lldb::TargetSP target_sp,
ListenerSP listener_sp,
- kvm_t *kvm,
const FileSpec &core_file)
- : PostMortemProcess(target_sp, listener_sp, core_file), m_kvm(kvm) {}
+ : PostMortemProcess(target_sp, listener_sp, core_file) {}
ProcessFreeBSDKernelCore::~ProcessFreeBSDKernelCore() {
if (m_kvm)
@@ -79,9 +78,11 @@ lldb::ProcessSP ProcessFreeBSDKernelCore::CreateInstance(
kvm_t *kvm =
kvm_open2(executable->GetFileSpec().GetPath().c_str(),
crash_file->GetPath().c_str(), O_RDONLY, nullptr, nullptr);
- if (kvm)
+ if (kvm) {
+ kvm_close(kvm);
return std::make_shared<ProcessFreeBSDKernelCore>(target_sp, listener_sp,
- kvm, *crash_file);
+ *crash_file);
+ }
}
return nullptr;
}
@@ -113,7 +114,21 @@ bool ProcessFreeBSDKernelCore::CanDebug(lldb::TargetSP target_sp,
}
Status ProcessFreeBSDKernelCore::DoLoadCore() {
- // The core is already loaded by CreateInstance().
+ ModuleSP executable = GetTarget().GetExecutableModule();
+ if (!executable)
+ return Status::FromErrorString(
+ "ProcessFreeBSDKernelCore: no executable module set on target");
+
+ m_kvm = kvm_open2(executable->GetFileSpec().GetPath().c_str(),
+ GetCoreFile().GetPath().c_str(), O_RDWR, nullptr, nullptr);
+
+ if (!m_kvm)
+ return Status::FromErrorStringWithFormat(
+ "ProcessFreeBSDKernelCore: kvm_open2 failed for core '%s' "
+ "with kernel '%s'",
+ GetCoreFile().GetPath().c_str(),
+ executable->GetFileSpec().GetPath().c_str());
+
SetKernelDisplacement();
return Status();
diff --git a/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.h b/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.h
index d82e55ea74bd9..77cb747fc95c1 100644
--- a/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.h
+++ b/lldb/source/Plugins/Process/FreeBSD-Kernel-Core/ProcessFreeBSDKernelCore.h
@@ -17,7 +17,7 @@
class ProcessFreeBSDKernelCore : public lldb_private::PostMortemProcess {
public:
ProcessFreeBSDKernelCore(lldb::TargetSP target_sp, lldb::ListenerSP listener,
- kvm_t *kvm, const lldb_private::FileSpec &core_file);
+ const lldb_private::FileSpec &core_file);
~ProcessFreeBSDKernelCore();
|
mchoo7
changed the title
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
Makes sense to me, DoLoadCore should.... do the loading of the core :)
If my guess at the cost of kvm_open2 is right, then this LGTM.
| @@ -79,9 +78,11 @@ lldb::ProcessSP ProcessFreeBSDKernelCore::CreateInstance( | |||
| kvm_t *kvm = | |||
| kvm_open2(executable->GetFileSpec().GetPath().c_str(), | |||
| crash_file->GetPath().c_str(), O_RDONLY, nullptr, nullptr); | |||
There was a problem hiding this comment.
From reading other ProcessElfCore::CreateInstance, my understanding is that they do some minimal work to ensure that the file is of the right type. Usually looking for magic bytes. Point is - something cheap.
The docs site is having issues so I can't read them at the moment, but my guess is that this kvm_open2 is opening a file descriptor for some /dev/mem and not parsing masses of data to do so.
Do these pointers need to be freed somehow?
There was a problem hiding this comment.
I see you close it with kvm_close.
There was a problem hiding this comment.
my guess is that this kvm_open2 is opening a file descriptor for some /dev/mem and not parsing masses of data to do so
kvm_open* will open /dev/mem for live debugging, or a core file, and do some validation and initialization. It'd be nice to avoid opening it twice, but it's not very expensive.
https://cgit.freebsd.org/src/tree/lib/libkvm/kvm.c#n113
https://cgit.freebsd.org/src/tree/lib/libkvm/kvm_minidump_amd64.c#n120
CC @bsdjhb who maintains our GCC kernel support for comment.
There was a problem hiding this comment.
Sounds fine then. I trust kvm to do the right validation more than I trust us to implement a subset of that validation correctly.
5 participants
ProcessFreeBSDKernelCoreinitializesm_kvmin class initialization, thus invokingkvm_open2()only once. Although this approach is effective, it violates the expected bahaviour ofDoLoadCore(), loading core file before the function is invoked. Later when I implement another flavour ofProcessFreeBSDKernelCoreinherited fromProcessElfCore, ELF plugin will load core inDoLoadCore()while kvm plugin will do so in the class initializer, causing discrepancy between the two classes. Like the kvm/fvc precedent, the plugin variant (ELF and kvm) will be chosen using vtable override, so if the behaviour differs like above, it gets harder to add new features and debug the code. Thus, detecting and loading core file inProcessFreeBSDKernelCoreshould be handled separately.